-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reduce boost regex dependencies #30333
Reduce boost regex dependencies #30333
Conversation
The code-checks are being triggered in jenkins. |
The code-checks are being triggered in jenkins. |
A new Pull Request was created by @camolezi (Lucas Camolezi) for master. It involves the following packages: CommonTools/Utils @perrotta, @smuzaffar, @benkrikler, @Dr15Jones, @alja, @makortel, @fwyzard, @schneiml, @andrius-k, @cmsbuild, @rekovic, @jfernan2, @kmaeshima, @fioriNTU, @Martin-Grunewald, @slava77, @ggovi, @santocch can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
+1 |
The tests are being triggered in jenkins.
|
@@ -13,7 +13,7 @@ | |||
#include "FWCore/Utilities/interface/Algorithms.h" | |||
|
|||
#include "boost/algorithm/string.hpp" | |||
#include "boost/regex.hpp" | |||
#include <regex> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file is obsolete. I suggest we let this change go in and then I remove the file (among others). Unless this PR needs to be modified for other reasons, in which case I'd ask to leave this file untouched (but only in that case).
+core |
-1 Tested at: c9f59b4 CMSSW: CMSSW_11_2_X_2020-06-22-1100 I found follow errors while testing this PR Failed tests: Build HeaderConsistency
I found compilation error when building: >> Package CondTools/SiPixel built >> Entering Package CondCore/AlignmentPlugins Entering library rule at src/CondCore/AlignmentPlugins/plugins >> Compiling edm plugin /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_2_X_2020-06-22-1100/src/CondCore/AlignmentPlugins/plugins/TrackerSurfaceDeformations_PayloadInspector.cc In file included from /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_2_X_2020-06-22-1100/src/CondCore/AlignmentPlugins/plugins/TrackerSurfaceDeformations_PayloadInspector.cc:12: /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_2_X_2020-06-22-1100/src/CondCore/Utilities/interface/PayloadInspector.h:253:12: error: 'set' in namespace 'std' does not name a template type std::set m_inputParams; ^~~ /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_2_X_2020-06-22-1100/src/CondCore/Utilities/interface/PayloadInspector.h:253:7: note: 'std::set' is defined in header ''; did you forget to '#include '? /data/cmsbld/jenkins/workspace/ib-run-pr-tests/CMSSW_11_2_X_2020-06-22-1100/src/CondCore/Utilities/interface/PayloadInspector.h:14:1: +#include |
Comparison not run due to Build errors (RelVals and Igprof tests were also skipped) |
@@ -3,7 +3,7 @@ | |||
#include <cstring> | |||
|
|||
// boost headers | |||
#include <boost/regex.hpp> | |||
#include <regex> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move this header to the "C++ headers" section, and remove the now empty "boost headers" section ?
@@ -3,7 +3,7 @@ | |||
#include <cstring> | |||
|
|||
// boost headers | |||
#include <boost/regex.hpp> | |||
#include <regex> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move this header to the "C++ headers" section, and remove the now empty "boost headers" section ?
@@ -4,7 +4,7 @@ | |||
#include <iomanip> | |||
#include <cassert> | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -3,7 +3,7 @@ | |||
|
|||
#include "CommonTools/Utils/interface/TFileDirectory.h" | |||
|
|||
#include "boost/regex.hpp" | |||
#include <regex> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move this header with the other standard headers above ?
@@ -13,7 +13,7 @@ | |||
#include <pwd.h> | |||
#include <climits> | |||
// |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// |
@@ -16,7 +16,7 @@ | |||
#include <vector> | |||
|
|||
// Boost Headers | |||
#include <boost/regex.hpp> | |||
#include <regex> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move this header with the other standard headers above ?
@@ -4,7 +4,7 @@ | |||
#include "EventFilter/L1TRawToDigi/interface/MP7FileReader.h" | |||
|
|||
#include <boost/algorithm/string.hpp> | |||
#include <boost/regex.hpp> | |||
#include <regex> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you move this header with the other standard headers below ?
I don't think we should go on with this replacement anymore, unless performance is not a concern. I've made some benchmarks on std::regex vs boost::regex. https://gist.github.com/camolezi/87283aaf7cdeb584ba294588a5d77abd |
@camolezi Interesting (and quick internet searches support that). I'd be curious if adding |
wow, It made a big difference. The performance difference is much more reasonable with -O2 flag. boost_regex: 2805ns - iterations: 250338 edit: it seems that std_regex is faster with -O2 flag. I benchmarked again a couple of times and this result seems right. |
Okay, my regex_match benchmarks do show that std_regex is comparable to boost regex. Based on this: It seems like in some specific cases std::regex does have a comparable performance to boost::regex. But in general, it seems like std::regex is significantly slower. |
Interesting - thanks a lot for the investigation. I am, of course, in favour of staying with the better-performing option :-) |
PR description:
Replaced boost regex for c++11 stl regex, they should have similar behavior.
PR validation:
Passed on basic runTheMatrix test.
if this PR is a backport please specify the original PR and why you need to backport that PR:
@vgvassilev @davidlange6